Skip to content

[gui] reenable fit panel testing and fix nullptr access#21882

Open
ferdymercury wants to merge 3 commits intoroot-project:masterfrom
ferdymercury:patch-20
Open

[gui] reenable fit panel testing and fix nullptr access#21882
ferdymercury wants to merge 3 commits intoroot-project:masterfrom
ferdymercury:patch-20

Conversation

@ferdymercury
Copy link
Copy Markdown
Collaborator

This Pull request:

Changes or fixes:

initialize to null instead

Fixes #21881

Checklist:

  • tested changes locally
  • updated the docs (if necessary)

@ferdymercury ferdymercury changed the title [asimage] prevent pointing to invalid memory [gui,asimage] prevent pointing to invalid memory Apr 10, 2026
@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 10, 2026

Test Results

    22 files      22 suites   3d 7h 14m 13s ⏱️
 3 834 tests  3 832 ✅  1 💤 1 ❌
76 575 runs  76 552 ✅ 18 💤 5 ❌

For more details on these failures, see this check.

Results for commit d32767c.

♻️ This comment has been updated with latest results.

if (values) {
fValues = *values;
fContext = gVirtualX->CreateGC(gVirtualX->GetDefaultRootWindow(), values);
if (gVirtualX)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking of gVirtualX does not make big sense - it always present, also in batch mode

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was null when i checked with the debugger. Note that TVirtualX::Instance can still return nullptr when on batch

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For changes in GUI classes we need ok from Bertrand @bellenot.
Can we combine this PR with enabling of gui test: #21891
Then we directly can see if our changes works/fails?

And finally we will be able to decide if such extra checks in GUI classes allows to use them in batch GUI tests.

@ferdymercury ferdymercury marked this pull request as ready for review April 13, 2026 08:55
@linev
Copy link
Copy Markdown
Member

linev commented Apr 13, 2026

I checkout your branch and try to run test.
There are many issues with normal GUI mode.

  1. Last two tests TestTree2D and TestTreeND do not work for me - while fit panel does not have entries like "gaus2d" or "gausND".
  2. Also running test in batch possible only with root -b --web=off UnitTesting.cxx. Produced test executable crashed much earlier because TGClient::Instance() returns nullptr
  3. When run inside root, one sees lot of errors about missing images.

@ferdymercury
Copy link
Copy Markdown
Collaborator Author

  • Last two tests TestTree2D and TestTreeND do not work for me - while fit panel does not have entries like "gaus2d" or "gausND".

Yes, same here, I had deactivated (commented) locally those old tests for the moment and was focusing on whether it was running at all without crashing.

  • Also running test in batch possible only with root -b --web=off UnitTesting.cxx. Produced test executable crashed much earlier because TGClient::Instance() returns nullptr

Weird, where? I see a different behavior on Linux. It just crashes when deleting the TGTooltip at shutdown for me.

  • When run inside root, one sees lot of errors about missing images.

Same here, that's what I would say "expected", a lot of errors in batch mode of not being able to load icons, but no crashes.

@linev
Copy link
Copy Markdown
Member

linev commented Apr 14, 2026

Weird, where? I see a different behavior on Linux. It just crashes when deleting the TGTooltip at shutdown for me.

I mean when I run compiled executable.

  • When run inside root, one sees lot of errors about missing images.

Same here, that's what I would say "expected", a lot of errors in batch mode of not being able to load icons, but no crashes.

This is most challenging part - try to understand if there double deletion or just wrong pointer released.
Because of lot of errors I cannot exclude incomplete initialization of some GUI classes

@linev
Copy link
Copy Markdown
Member

linev commented Apr 14, 2026

@ferdymercury @bellenot

I probably found the reason why it crashes.
Seems to be it is wrongly set flag TGLabel::fHasOwnFont.
In batch it is incorrect and TGLabel destructor tries to free TGGC object which does not belong to the label.

Also side problem is that in TGGC::TGGC(GCValues_t *values) constructor ref counter not always set correctly.

Fixing this two problems makes test running for me

@linev
Copy link
Copy Markdown
Member

linev commented Apr 14, 2026

First step is #21907

@ferdymercury
Can you extract your changes in TASImage to separate PR.

After such two steps we will be able to enable this and may be other GUI tests in CI

@ferdymercury ferdymercury changed the title [gui,asimage] prevent pointing to invalid memory [gui] reenable fit panel testing and fix nullptr access Apr 14, 2026
Comment on lines +83 to +84
if (gVirtualX)
gVirtualX->ChangeWindowAttributes(fId, &wattr);
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably these changes are no longer needed since I was trying out different things and you already found the culprit in TGGC

@linev
Copy link
Copy Markdown
Member

linev commented Apr 15, 2026

@ferdymercury

After #21907 is merged you can try rebase your code and see if now fit panel test can be enabled.

@linev
Copy link
Copy Markdown
Member

linev commented Apr 15, 2026

@ferdymercury

Please remove all changes in gui classes from this PR.

And at the end of UnitTesting.cxx one should have:

int UnitTesting()
{
   gROOT->SetWebDisplay("off");

   FitEditorUnitTesting fUT;

   return fUT.UnitTesting();
}

// The main function. It is VERY important that it is run using the
// TApplication.
int main(int argc, char** argv)
{
   TApplication theApp("App",&argc,argv);

   // force creation of client
   if (!TGClient::Instance())
      new TGClient();

   int ret = UnitTesting();

   theApp.Terminate();

   return ret;
}

And please check TestTree1D. It fails while reference values for fir parameters do not match.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[gui,asimage] crash when opening xpm icon in batch mode

2 participants